Thank you!

Change the code to simply drop unexpected PDUs (as opposed to attempting to replay the unmatched leftover, which isn't clearly needed)

Sounds reasonable!

This require Acceptor::step() to be idempotent on error, so we have to clone the state fields, instead of taking the state.

To be honest, I’m not very inclined into making this change, as I believe there is a better approach.

Before going further, I want to check my understanding at this place: https://github.com/Devolutions/IronRDP/pull/630/files#r1905600306

Hi, sorry for the delay.

I gave a bit more of thought to this, and it’s indeed not so simple to do better as I initially thought.

Let me go over the problem we are solving, and reformulate my concerns and questions I have with the way it’s implemented now.

We are trying to be as resilient as possible by ignoring anything we don’t expect for the current accept sequence state. This means:

• Things that are not matching the provided hint.
• Things that can’t be decoded into the expected structure.

This happens because as I understand it, the client does not stop sending active session packets (e.g.: FastPathInput PDUs, etc) even though the deact-react sequence is already started.

Ideally, we would rely on some synchronization logic, a "fence", to agree with the client on when packets relevant for the reactivation (~= connection/accept) logic should be send and received.

In the absence of such synchronization logic, we need to run the sequence state machine, without consuming its state even if we failed at decoding the expected structure from the provided frame. This results into having to clone everything because of the way we are consuming the current state to turn it into the next one.

Here are some of my concerns:

• It’s not very clean to clone our way through this. I acknowledge that this is a possible fix, and I would be okay to merge this as a stopgap solution if you think it’s the best we can do with minimal efforts.

• Typically, servers are just killing the connection brutally when the client is behaving in some weird way. Microsoft’s official server is doing this. I feel we are doing something wrong if we need to accept what the official server would usually reject.

• From the above: instead of terminating the connection as soon as we get something weird, we ignore the problem, and the session may stay hanging around, doing nothing useful for a while.

I investigated the RDP deact-react sequence more in depth, to better understand the domain and possible reasons that may be leading to this problem in the first place.

Normally, the deact-react sequence works as follows:

• The server initiates the deactivation. When the server needs to tear down the current share or reconfigure it, by sending a Deactivate All PDU.

• Upon reception of the Deactivate All PDU, the client must stop sending active-share graphics updates (both FastPath updates and SlowPath updates).

• The server begins reactivation by sending a new Demand Active PDU, which carries the new share settings.

• The client then responds with Confirm Active (and possibly other PDUs like Synchronize, Control, Font List, etc.).

The key point is that once the server has sent Deactivate All, the client is expected to stop sending further PDUs refering to the old context, and wait for the new handshake. In principle, it should be fine to just fail upon reception of an unexpected payload.

At this point, I suspect we may be hitting a race condition depending on the network latency with client’s send buffer hitting the wire even after the server decided to deactivate, and by the time these packets arrive, the server has already started the reactivation process. Or, there is a client-side bug. For instance, I’m not sure if the IronRDP client is properly adhering to the deactivation order sent by the server.

Best approach:

• Fix IronRDP client if it does not properly follow the logic to quiesce old PDUs on Deactivate All.
• Send the Deactivate All and Demande Active PDUs to the client, and wait for the Confirm Active PDU. Any PDU received in between should be discarded.

The key point to this approach is that we just need to parse received frames until a Confirm Active PDU is received, and then we can drive the sequence as usual.

In pseudo-code:

let frame = loop {
    let frame = read_frame_by_hint();
    
    match decode_confirm_active_pdu(frame) {
        Ok(_) => break frame,
        Err(DecodError) => {}
        Err(error) => return Err(error),
    }
};

// Feed the frame, which is just the PDU that is expected for the next step.
// We decode the frame twice, but from this point on we don’t need to check for the decode result again.
accept_sequence.step(frame);

// At this point, drive the sequence as usual.

=> We pump and discard all the messages exchanged between the Deactivate All and the Confirm Active PDUs.

Less ideal approach:

If we really need the ability of handling unexpected PDUs at any point in the accept sequence, then maybe a better approach is to try decoding the next expected PDU for the current state before actually executing the step. We may need to add an extra trait in the ironrdp-acceptor crate for this purpose.

Less ideal approach 2:

Move data out of the state variants, and use Options directly at the ClientConnector so that we can set and take the various states without cloning everything, and it’s possible to leave the state machine untouched until the expected PDU is received.

Extra remark:

Definitely something for a separate PR, but we may want a timeout during the connection and deact-react sequences, in case the client decides to do something akin to a “slow” or “lazy” DoS. An RDP server should never be exposed to the internet so this is debatable whether an attack could actually take place though.

Do you think the "best approach" (as I call it) would work?
Would you prefer that we still merge this PR as-is and improve on it afterwards?

I’m open to any of your suggestions at this point.

LGTM! Thank you!!